feat: add Map and Set sanitization via sanitizeCollections option#305
Conversation
📝 WalkthroughWalkthroughThis PR implements an opt-in ChangesMap and Set Sanitization Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/replacers.test.ts (1)
1128-1128: ⚡ Quick winUse
DEFAULT_PATTERN_MASKinstead of hardcoded mask literals.These two assertions hardcode
'**********', which makes tests brittle if the default mask changes.Proposed fix
- expect(sanitized.get('message')).toBe('api_key=**********'); + expect(sanitized.get('message')).toBe( + `api_key=${DEFAULT_PATTERN_MASK}`, + ); - expect(sanitized.has('api_key=**********')).toBe(true); + expect( + sanitized.has(`api_key=${DEFAULT_PATTERN_MASK}`), + ).toBe(true);Also applies to: 1231-1231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/replacers.test.ts` at line 1128, The test is asserting a hardcoded mask string ('**********') which should use the shared DEFAULT_PATTERN_MASK constant to avoid brittleness; update the two assertions that check sanitized.get('message') (and the other occurrence at the second mentioned location) to compare against DEFAULT_PATTERN_MASK instead of the literal '**********' so the test follows the current default mask value (import or reference DEFAULT_PATTERN_MASK in tests if not already imported).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ROADMAP.md`:
- Around line 147-148: Update the "Collect Usage Signals" section in ROADMAP.md
to remove or rephrase the guidance that says you must gather usage signals
before planning Map/Set support, since Map and Set are now marked implemented in
the table; locate the table rows referencing "Map" and "Set" and the "Collect
Usage Signals" heading and change the phrasing to acknowledge Map/Set are
implemented (for example, note they are implemented and optional via
sanitizeCollections: true or suggest monitoring post-release usage instead of
pre-planning signal collection).
---
Nitpick comments:
In `@test/replacers.test.ts`:
- Line 1128: The test is asserting a hardcoded mask string ('**********') which
should use the shared DEFAULT_PATTERN_MASK constant to avoid brittleness; update
the two assertions that check sanitized.get('message') (and the other occurrence
at the second mentioned location) to compare against DEFAULT_PATTERN_MASK
instead of the literal '**********' so the test follows the current default mask
value (import or reference DEFAULT_PATTERN_MASK in tests if not already
imported).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 831f4423-c8d4-4009-9dae-6d101b51e606
📒 Files selected for processing (7)
README.mdbench/sanitize-data.bench.tsdocs/ROADMAP.mddocs/plans/012-map-set-sanitization.mdsrc/replacers.tssrc/types.tstest/replacers.test.ts
…ge signals to class instances
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/replacers.test.ts (2)
1194-1274: 💤 Low valueOptionally add tests for
removeMatchesandscanStringValues: falsewith Sets.For additional coverage completeness:
removeMatches with Set string scanning — When a Set contains
'api_key=hunter2'andremoveMatches: true, verify the behavior (likely produces empty string from the scan-and-remove logic).scanStringValues: false with Set — Verify that when
scanStringValues: false, string items in Sets are not scanned for embedded patterns, consistent with plain object behavior (tested at line 956-973).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/replacers.test.ts` around lines 1194 - 1274, Add two new tests under the existing "Set sanitization" describe that call objectReplacer with sanitizeCollections: true and reference DEFAULT_PATTERN_MASK: one test sets removeMatches: true and a Set containing 'api_key=hunter2' and asserts the Set item is removed or becomes the expected empty/removed-result per the scan-and-remove logic (use removeMatches flag passed to objectReplacer); the other test sets scanStringValues: false with a Set containing 'api_key=hunter2' and asserts the string is left unchanged (i.e., still 'api_key=hunter2'), mirroring the behavior asserted for plain objects; locate usages near the other Set tests so they use the same patterns and assertions.
1036-1274: ⚡ Quick winConsider adding tests for circular references and non-plain object preservation.
The Map and Set test coverage is solid for the main scenarios. However, two important edge cases are missing that align with the coding guidelines:
Circular reference detection — The implementation uses
WeakSetto detect circular references and throwsTypeError. Add tests like:const circularMap = new Map(); circularMap.set('self', circularMap); expect(() => objectReplacer({ data: circularMap }, { sanitizeCollections: true })) .toThrow(TypeError);Non-plain object preservation in collections — Per guidelines, non-plain objects (e.g.,
Date, custom classes) should pass through unchanged. The test at line 754-796 verifies this whensanitizeCollectionsisfalse, but not when it'strue. Add tests like:const date = new Date('2024-01-01'); const map = new Map([['timestamp', date], ['username', 'mark']]); const result = objectReplacer({ map }, { sanitizeCollections: true }); const sanitized = result.map as Map<string, unknown>; expect(sanitized.get('timestamp')).toBe(date); // same instanceAs per coding guidelines, object replacers must detect circular references and preserve non-plain object instances.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/replacers.test.ts` around lines 1036 - 1274, Add two tests for collection handling: (1) verify circular reference detection by creating a Map/Set that contains itself and asserting objectReplacer({ data: circular }, { sanitizeCollections: true }) throws a TypeError (this exercises the WeakSet-based cycle detection inside objectReplacer); (2) verify non-plain objects are preserved when sanitizeCollections is true by putting a Date (or custom class instance) into a Map/Set alongside a normal key and asserting the sanitized collection contains the exact same Date instance (not cloned or modified) while other sensitive entries are masked; place these alongside the existing Map/Set tests referencing objectReplacer and the sanitizeCollections option.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/replacers.test.ts`:
- Around line 1194-1274: Add two new tests under the existing "Set sanitization"
describe that call objectReplacer with sanitizeCollections: true and reference
DEFAULT_PATTERN_MASK: one test sets removeMatches: true and a Set containing
'api_key=hunter2' and asserts the Set item is removed or becomes the expected
empty/removed-result per the scan-and-remove logic (use removeMatches flag
passed to objectReplacer); the other test sets scanStringValues: false with a
Set containing 'api_key=hunter2' and asserts the string is left unchanged (i.e.,
still 'api_key=hunter2'), mirroring the behavior asserted for plain objects;
locate usages near the other Set tests so they use the same patterns and
assertions.
- Around line 1036-1274: Add two tests for collection handling: (1) verify
circular reference detection by creating a Map/Set that contains itself and
asserting objectReplacer({ data: circular }, { sanitizeCollections: true })
throws a TypeError (this exercises the WeakSet-based cycle detection inside
objectReplacer); (2) verify non-plain objects are preserved when
sanitizeCollections is true by putting a Date (or custom class instance) into a
Map/Set alongside a normal key and asserting the sanitized collection contains
the exact same Date instance (not cloned or modified) while other sensitive
entries are masked; place these alongside the existing Map/Set tests referencing
objectReplacer and the sanitizeCollections option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c57f9b6-ec29-4f2b-aa7b-7785e092978d
📒 Files selected for processing (2)
docs/ROADMAP.mdtest/replacers.test.ts
Overview
Adds opt-in
MapandSetsanitization via a newsanitizeCollectionsoption (defaultfalse). When enabled,objectReplacertraverses each collection and returns a new sanitized copy — the original is never mutated.Details
sanitizeCollections?: booleanadded toDataSanitizationReplacerOptionsMapentries: string keys are matched against field-name patterns; matched values are masked or removed. Object keys are recursed into and sanitized. String keys themselves are not sanitized (consistent with plain object property name behaviour).Setvalues: each item is recursed throughsanitizeValue— strings are scanned for embedded patterns, objects are sanitized recursively.falseso existing pass-through behaviour for non-plain objects is preserved.bench/sanitize-data.bench.ts(Map shallow + Set small).Related Tickets and/or Pull Requests
Relates to plan 012
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Benchmarks